Restore: Move to trash button in Document settings #65087
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +295 B (+0.02%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 81d9da3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10718949568
|
|
Popular button, let's restore it. We should change the spacing a little bit, make the space above trash button a bit larger. Technically it's correct at 4px: But optically it will feel more balanced if it uses this gap: And so either you can move that button out of the list so it gets the correct 16px gap between sections, or you can just add a 12px top margin to the button:
|
jasmussen
approved these changes
Sep 5, 2024
youknowriad
reviewed
Sep 5, 2024
| @@ -86,6 +87,7 @@ export default function PostSummary( { onActionPerformed } ) { | |||
| <PostsPerPage /> | |||
| <SiteDiscussion /> | |||
| <PostFormatPanel /> | |||
| <PostTrash /> | |||
There was a problem hiding this comment.
So I'm guessing @jasmussen is suggesting we move it outside of the VStack before the fills
There was a problem hiding this comment.
Yeah let's move it outside of the VStack so it inherits the 16px gap.
There was a problem hiding this comment.
The PostTrashCheck wrapper is missing here.
youknowriad
reviewed
Sep 5, 2024
| permissions, | ||
| }; | ||
| }, [ item, permissions ] ); | ||
| const actions = usePostActions( { postType } ); |
There was a problem hiding this comment.
It feels way too heavy for me to call usePostActions just to check if one single action is illigible or not. I'd rather just duplicate the check.
There was a problem hiding this comment.
Hi @youknowriad, I updated the code and now we don't call usePostActions on eligibility check.
youknowriad
reviewed
Sep 5, 2024
| @@ -1,71 +1,84 @@ | |||
| /** | |||
| * WordPress dependencies | |||
| */ | |||
| import { __ } from '@wordpress/i18n'; | |||
There was a problem hiding this comment.
Why did we refactor this component? What was wrong with the previous implementation?
There was a problem hiding this comment.
We should not have two different modals to trash, we should rely on the one the actions already offer.
E.g: less strings to translate. The UI when pressing trash on actions and button also becomes the same. It is unexpected to get one UI if I press it in a given way and different UI if I press it in other way.
There was a problem hiding this comment.
This update also handles cases where a plugin removes an action, such as the move to trash action. With these changes, we also don't render this button if the action is removed.
There was a problem hiding this comment.
I also don't see a reason for this change. Original components are well-tested, and we're not introducing any new code, so "duplication" is justified.
If the modals don't match, we can just update the confirmation modal in the PostTrash component.
There was a problem hiding this comment.
@Mamaduka as referred above we have API's to remove actions unregisterEntityAction, so I expect that if I unregister the move to trash action this button goes away. It becomes inconsistent that the actions go away on the actions menu but not on this button. If we need to retrieve the action for this check why not just use the modal the action provides?
There was a problem hiding this comment.
I agree that reuse the actions would be good, but my opinion is that the way we're reusing them now (with an adhoc component within the editor package and not exported from the dataviews package) is not good.
I think Actions are a dataviews API and should ideally not be used in other things that are not dataviews components.
There was a problem hiding this comment.
@jorgefilipecosta, the unregisterEntityAction controls one part of the UI - entity actions. I don't expect them to affect other parts.
E.g., If someone removes the "Rename" action, should we also hide the Post Title field in the canvas?
There was a problem hiding this comment.
Fair points, I followed the suggestions and updated to keep the previous logic, but changed the modal to the be similar to the actions one.
|
Hi @jasmussen I moved the button outside of the VStack before the fills as suggested. @Mamaduka @youknowriad I refactored the button and checks to be based on the actions, it calls the same as the action and the check is based on action.isEligible. |
These components are public API. I think the most straightforward approach here is to restore functionality using their original code. |
jorgefilipecosta
force-pushed
the
readd/move-to-trash-button
branch
from
eb18890 to
e0769fc
Compare
|
Hi @youknowriad, @Mamaduka I updated the code to following your feedback. |
Mamaduka
approved these changes
Sep 5, 2024
There was a problem hiding this comment.
Thank you, @jorgefilipecosta!
Left a small note regarding the wp_navigation post type, but besides that, everything works as expected!
Comment on lines
40
to
42
| postType !== 'wp_template' && | ||
| postType !== 'wp_block' && | ||
| postType !== 'wp_template_part', |
There was a problem hiding this comment.
I think wp_navigation should also be here. You could reuse GLOBAL_POST_TYPES from store.
gutenberg/packages/editor/src/store/constants.ts
Lines 32 to 35 in 57c59d1
Mamaduka
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #65023
This PR readds a big "Move to trash" button that was available on older releases and was meanwhile removed.
In progress:
Needs to call the action that already has a modal instead of implementing its own confirmation modal.
Should use eligibility checks of the action.
Should not appear when editing the template of a post on the post editor.
Screenshots or screencast